[rl] RendererConfig silently drops the removed preserve_* renderer knobs by andylizf · Pull Request #4365 · pytorch/torchtitan

andylizf · 2026-08-28T06:05:51Z

RendererConfig.build forwards knobs to the renderers config by matching field
names, and drops anything that does not match:

args = {
    field.name: getattr(self, field.name)
    for field in fields(self)
    if field.name != "name"
    and getattr(self, field.name) is not None
    and field.name in config_type.model_fields
}

renderers replaced preserve_all_thinking and preserve_thinking_between_tool_calls
with a single thinking_retention enum in PrimeIntellect-ai/renderers#88 (2026-06-26).
Neither name is a field on a renderer config any more, so both are dropped. Both are
still declared on RendererConfig and documented in its docstring, and
components/training_sample_builder.py recommends setting preserve_all_thinking=True
to keep thinking from being stripped out of history. experiments/rl/requirements.txt
tracks renderers at main, so the knobs have been inert since that commit.

The drop is silent, and it defeats a guard the library added for this case: renderers
keeps a validator that raises "[...] were replaced by thinking_retention" when an old
name reaches it, and the typed configs use extra="forbid". The name filter removes the
field before either can see it. Startup logs the result without flagging it:

Using renderer qwen3.5, of type <class 'renderers.configs.Qwen35RendererConfig'>, with args {}

Where we hit it: a recipe setting preserve_all_thinking=True ran under the renderer's
implied tool_cycle policy instead. For an agent loop that returns tool output as plain
user messages, every turn reads as a new user query, so bridge_to_next_turn refuses to
continue and the trajectory is re-rendered from scratch — 40694 of 40697 turns in one
run, against a code comment that expects zero. On a hybrid model without prefix caching
that is a full recompute per turn.

This PR:

Adds thinking_retention to RendererConfig so callers have a working path, typed
as the library's Literal["tool_cycle", "all"].
Raises when a removed knob is set to True and the resolved config cannot take it,
naming the replacement. An explicit False is left alone: it meant "defer to the
chat template", which is what dropping it already does.
Forwards thinking_retention on the auto path by constructing AutoRendererConfig,
which the library provides precisely to carry it through tokenizer-based resolution.
Other knobs set there still reach nothing, so they raise rather than being ignored,
and create_renderer(tokenizer, None) is kept unchanged when nothing is set.

Verified against the installed renderers: thinking_retention through auto resolves to
the same config object as naming the renderer explicitly, across all 53 models in
MODEL_RENDERER_MAP; create_renderer(tokenizer, None) and
create_renderer(tokenizer, AutoRendererConfig()) are equivalent; a matrix over names
(None, "auto", "qwen3", unknown) x both legacy fields (None/False/True) x
retention set/unset behaves as described; and existing callers that set only name and
enable_thinking, or pass tool_parser alongside a model-specific renderer, are
unaffected.

One gap left alone: config_from_name() still runs before the removed-knob check, so an
unknown renderer name reports that first and masks the legacy message. I can reorder it
if you would rather the legacy message win.

…tention through auto renderers replaced preserve_all_thinking and preserve_thinking_between_tool_calls with a single thinking_retention enum (PrimeIntellect-ai/renderers#88). Neither name is a field on a renderer config any more, so RendererConfig.build, which forwards by matching field names, drops both without a word -- and drops them before the library's own validator, which raises and names the replacement, can see them. Forward thinking_retention as its own field, and raise when a removed knob is set to True and the resolved config cannot take it. On the auto path (name=None or "auto") the library's AutoRendererConfig carries thinking_retention into the resolved config and nothing else, so forward that one knob there and raise for the rest instead of dropping them. An explicit False is left alone: it meant "defer to the chat template", which is what dropping it does.

meta-cla · 2026-08-28T06:05:56Z

Hi @andylizf!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

pytorch-bot · 2026-08-28T06:05:58Z

The following ciflow label(s) have been added but CI has not been triggered yet because the workflows are awaiting approval:

ciflow/rl

Once a maintainer approves the workflows (scroll to the bottom of the PR page), the corresponding CI jobs will be triggered automatically. Please ping one of the reviewers if you do not have access to approve and run workflows.

felipemello1 · 2026-08-29T00:55:11Z

hey @andylizf , thanks for the PR! I am on PTO until Monday. I will try to review over the weekend. If not, i will do it on Tuesday

felipemello1 · 2026-08-29T01:20:19Z

        enable_thinking: Let the model emit reasoning, when supported.
-        preserve_all_thinking: Keep historical reasoning in future prompts.
-        preserve_thinking_between_tool_calls: Keep reasoning during tool loops.
+        preserve_all_thinking: Removed upstream; see `thinking_retention`.


we dont really do backward compatibility. If this is a dead knob, lets remove it

felipemello1 · 2026-08-29T01:21:02Z

        renderer_name = _RENDERER_BY_MODEL.get(self.name, self.name)
        renderer_config = config_from_name(renderer_name) if renderer_name else None
+
+        # `renderers` replaced these two bools with the single `thinking_retention`


same here, no need to keep comments about historical motivation

felipemello1 · 2026-08-29T01:24:09Z

@claude please review. What is a robust and elegant/simple way of forwarding configs without silently allowing errors like this? please think of 3 ideas with code snippets. Dont be verbose.

claude · 2026-08-29T01:24:36Z

Claude finished @felipemello1's task in 46s —— View job

Forwarding configs without silent drops

The root cause is the field.name in config_type.model_fields filter: it turns "knob the target doesn't accept" into "silently dropped." renderers already ships the guard we want (extra="forbid" + a validator that names the thinking_retention replacement). So the most robust move is to stop filtering and let the target reject unknowns.

Idea 1 — Delegate to the library's own guard (simplest, my pick).
Drop the membership filter; pass every set knob through. extra="forbid" and the removed-knob validator do the work, and any future rename fails loudly for free.

args = {
    f.name: getattr(self, f.name)
    for f in fields(self)
    if f.name != "name" and getattr(self, f.name) is not None
}
# config_type has extra="forbid" + a validator naming thinking_retention,
# so a removed/typo'd knob raises here instead of vanishing.
return create_renderer(tokenizer, config_type(**args))

Idea 2 — Don't mirror the schema at all.
Mirroring renderer fields on RendererConfig is what created the drift; the library is the source of truth. Hold a pass-through dict (or the typed sub-config) instead.

name: str | None = None
options: dict[str, Any] = field(default_factory=dict)  # forwarded verbatim
...
config_type(**self.options)  # unknown keys -> pydantic raises

Trade-off: loses per-field typing/discoverability in TorchTitan's config. Good when the wrapped schema churns often.

Idea 3 — Explicit diff, if you want to keep the typed mirror.
Keep the named fields for discoverability, but assert the mirror stays in sync instead of filtering. This is the generalization of what the PR already does for the two removed bools — applied to all knobs, so it works even without extra="forbid" and names the target.

provided = {f.name for f in fields(self)
            if f.name != "name" and getattr(self, f.name) is not None}
unknown = provided - config_type.model_fields.keys()
if unknown:
    raise ValueError(f"{sorted(unknown)} not accepted by {config_type.__name__}")

Recommendation: Idea 1. It deletes code, removes the mirror-vs-library drift entirely, and reuses the guard renderers built for exactly this. Combined with @felipemello1's two review notes — since we don't do back-compat, delete preserve_all_thinking / preserve_thinking_between_tool_calls and their historical comments rather than special-casing them — the whole removed-knob block collapses into the one-liner above; the auto path still needs its explicit thinking_retention forwarding since AutoRendererConfig is the only thing that carries it through tokenizer resolution.

Want me to push this simplification to the branch?
· fix-renderer-knob-drop

…d renderer knobs strictly renderers 0.1.11 makes transformers optional and accepts a bring-your-own tokenizer, so RendererConfig.build now takes TorchTitan's HuggingFaceTokenizer (wrapped in RendererTokenizer to satisfy renderers.OffsetTokenizer) instead of loading transformers.AutoTokenizer. The controller reads pad_id off its own tokenizer instead of renderer._tokenizer. RendererConfig forwards every set knob to the renderer's typed pydantic config without filtering by field name, so a knob the renderer lacks raises (extra="forbid") instead of being silently dropped (pytorch#4365). The removed preserve_* knobs and the DefaultRenderer-only tool_parser/reasoning_parser are deleted; thinking_retention is added. Renderer names are the library's names (_RENDERER_BY_MODEL removed). Two configs that silently lost enable_thinking today (gpt-oss alphabet sort, muse_glimmer search_r1) drop the knob. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…enderer options through renderers 0.1.11 makes transformers optional and accepts a bring-your-own tokenizer, so RendererConfig.build now takes TorchTitan's HuggingFaceTokenizer (wrapped in RendererTokenizer to satisfy renderers.OffsetTokenizer) instead of loading transformers.AutoTokenizer. The controller reads pad_id off its own tokenizer instead of renderer._tokenizer. RendererConfig is now `name` + `options`: the options dict is passed unchanged to the renderer's typed pydantic config, which rejects options that renderer does not have (extra="forbid") instead of TorchTitan silently dropping them (pytorch#4365). TorchTitan no longer mirrors renderer fields; the removed preserve_* knobs and the DefaultRenderer-only tool_parser/reasoning_parser are gone, and renderer names are the library's names (_RENDERER_BY_MODEL removed). Recipes move to options={...}; the gpt-oss alphabet-sort recipe, whose enable_thinking=False was being dropped, now sets reasoning_effort="low". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…he renderer's typed config directly renderers 0.1.11 makes transformers optional and accepts a bring-your-own tokenizer. RL now renders with TorchTitan's HuggingFaceTokenizer (wrapped in RendererTokenizer to satisfy renderers.OffsetTokenizer) instead of loading transformers.AutoTokenizer, and the controller reads pad_id off its own tokenizer instead of renderer._tokenizer. TorchTitan's RendererConfig wrapper is removed. Controller.Config.renderer is the library's own typed pydantic config (Qwen3RendererConfig(enable_thinking=False), GptOssRendererConfig(reasoning_effort="low"), ...), so a wrong option fails when the recipe is constructed instead of being silently dropped (pytorch#4365), and TorchTitan mirrors no renderer fields. build_renderer(tokenizer, config) is the one TorchTitan-side seam; Muse Glimmer's in-tree renderer registers itself when its module is imported, which unpickling its config in a rollout worker does. Renderer options are set in the recipe (tyro.conf.Suppress, like model_spec); the 8 redundant --renderer.enable-thinking CLI flags in the integration tests are removed. Configurable.to_dict learns pydantic model_dump so the wandb config stays JSON. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…he renderer's typed config directly renderers 0.1.11 makes transformers optional and accepts a bring-your-own tokenizer. RL now renders with TorchTitan's HuggingFaceTokenizer (wrapped in RendererTokenizer to satisfy renderers.OffsetTokenizer) instead of loading transformers.AutoTokenizer, and the controller reads pad_id off its own tokenizer instead of renderer._tokenizer. TorchTitan's RendererConfig wrapper is removed. Controller.Config.renderer is the library's own typed pydantic config (Qwen3RendererConfig(enable_thinking=False), GptOssRendererConfig(reasoning_effort="low"), ...), so a wrong option fails when the recipe is constructed instead of being silently dropped (pytorch#4365), and TorchTitan mirrors no renderer fields. build_renderer(tokenizer, config) is the one TorchTitan-side seam. A renderer that ships in TorchTitan (Muse Glimmer) has an InTreeRendererConfig that names its renderer class, and build_renderer constructs it directly, so nothing is written into renderers' registry. AutoRendererConfig and DefaultRendererConfig are rejected with the reason: the former never matches a local checkpoint path, the latter needs Hugging Face's apply_chat_template, which TorchTitan's template rendering does not match. Renderer options are set in the recipe (tyro.conf.Suppress, like model_spec); the 8 redundant --renderer.enable-thinking CLI flags in the integration tests are removed. Configurable.to_dict learns pydantic model_dump so the wandb config stays JSON. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…he renderer's typed config directly renderers 0.1.11 makes transformers optional and accepts a bring-your-own tokenizer. RL now renders with TorchTitan's HuggingFaceTokenizer (wrapped in RendererTokenizer to satisfy renderers.OffsetTokenizer) instead of loading transformers.AutoTokenizer, and the controller reads pad_id off its own tokenizer instead of renderer._tokenizer. TorchTitan's RendererConfig wrapper is removed. Controller.Config.renderer is the library's own typed pydantic config (Qwen3RendererConfig(enable_thinking=False), GptOssRendererConfig(reasoning_effort="low"), ...), so a wrong option fails when the recipe is constructed instead of being silently dropped (pytorch#4365), and TorchTitan mirrors no renderer fields. build_renderer(tokenizer, config) is the one TorchTitan-side seam. A renderer that ships in TorchTitan (Muse Glimmer) has an InTreeRendererConfig that names its renderer class, and build_renderer constructs it directly, so nothing is written into renderers' registry. AutoRendererConfig and DefaultRendererConfig are rejected with the reason: the former depends on an exact model-ID match that local asset paths do not reliably preserve; the latter needs Hugging Face-compatible apply_chat_template semantics, which TorchTitan's template rendering does not provide. Renderer options are set in the recipe (tyro.conf.Suppress, like model_spec); the 8 redundant --renderer.enable-thinking CLI flags in the integration tests are removed. Configurable.to_dict learns pydantic model_dump so the wandb config stays JSON. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…he renderer's typed config directly renderers 0.1.11 makes transformers optional and accepts a bring-your-own tokenizer. RL now renders with TorchTitan's HuggingFaceTokenizer (wrapped in RendererTokenizer to satisfy renderers.OffsetTokenizer) instead of loading transformers.AutoTokenizer, and the controller reads pad_id off its own tokenizer instead of renderer._tokenizer. TorchTitan's RendererConfig wrapper is removed. Controller.Config.renderer is the library's own typed pydantic config (Qwen3RendererConfig(enable_thinking=False), GptOssRendererConfig(reasoning_effort="low"), ...), so a wrong option fails when the recipe is constructed instead of being silently dropped (pytorch#4365), and TorchTitan mirrors no renderer fields. build_renderer(tokenizer, config) is the one TorchTitan-side seam. A renderer that ships in TorchTitan (Muse Glimmer) has a TorchTitanRendererConfig that names its renderer class, and build_renderer constructs it directly, so nothing is written into renderers' registry. AutoRendererConfig and DefaultRendererConfig are rejected with the reason: the former depends on an exact model-ID match that local asset paths do not reliably preserve; the latter needs Hugging Face-compatible apply_chat_template semantics, which TorchTitan's template rendering does not provide. Renderer options are set in the recipe (tyro.conf.Suppress, like model_spec); the 8 redundant --renderer.enable-thinking CLI flags in the integration tests are removed. Configurable.to_dict learns pydantic model_dump so the wandb config stays JSON. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

andylizf added 3 commits August 28, 2026 00:51

[rl] note that auto now carries thinking_retention

f5d7851

[rl] leave an explicit preserve_*=False alone on the auto path

4535375

pytorch-bot Bot added the ciflow/rl label Aug 28, 2026

tianyu-l requested a review from felipemello1 August 28, 2026 07:34

tianyu-l assigned felipemello1 Aug 28, 2026

meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 28, 2026

felipemello1 reviewed Aug 29, 2026

View reviewed changes

felipemello1 mentioned this pull request Sep 1, 2026

[rl] Pin renderers 0.1.11, render with TorchTitan's tokenizer, forward renderer knobs strictly felipemello1/torchtitan#42

Open

felipemello1 mentioned this pull request Sep 2, 2026

[rl] Add optional Verifiers rollout integration #4356

Open

felipemello1 mentioned this pull request Sep 2, 2026

[rl] Pin renderers 0.1.11, render with TorchTitan's tokenizer, take the renderer's typed config directly felipemello1/torchtitan#43

Open

felipemello1 mentioned this pull request Sep 3, 2026

[rl] Pin renderers 0.1.11, render with TorchTitan's tokenizer, take the renderer's typed config directly #4444

Open

Navigation Menu

Uh oh!

[rl] RendererConfig silently drops the removed preserve_* renderer knobs - #4365

[rl] RendererConfig silently drops the removed preserve_* renderer knobs#4365
andylizf wants to merge 3 commits into
pytorch:mainfrom
andylizf:fix-renderer-knob-drop

andylizf commented Aug 28, 2026

Uh oh!

meta-cla Bot commented Aug 28, 2026

Uh oh!

pytorch-bot Bot commented Aug 28, 2026

Uh oh!

felipemello1 commented Aug 29, 2026

Uh oh!

felipemello1 Aug 29, 2026

Uh oh!

felipemello1 Aug 29, 2026

Uh oh!

felipemello1 commented Aug 29, 2026

Uh oh!

claude Bot commented Aug 29, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

andylizf commented Aug 28, 2026

Uh oh!

meta-cla Bot commented Aug 28, 2026

Action Required

Process

Uh oh!

pytorch-bot Bot commented Aug 28, 2026

Uh oh!

felipemello1 commented Aug 29, 2026

Uh oh!

felipemello1 Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

felipemello1 Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

felipemello1 commented Aug 29, 2026

Uh oh!

claude Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Forwarding configs without silent drops

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

claude Bot commented Aug 29, 2026 •

edited

Loading